Simplify model launcher configs and add script input checks #90
Conversation
…configs thorugh adding hierarchical defaults
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -2,7 +2,7 @@ defaults: | |||
| - default | |||
| - _self_ | |||
|
|
|||
| input_dir: ${output_cohort_dir}/data | |||
There was a problem hiding this comment.
Is MEDS_cohort_dir used for anything else? If not, do we need to specify it and input_dir separately? Can we just have one parameter, which will help avoid the confusion that comes about in the setting where you are or aren't using a resharding stage (b/c when you are using a re-sharding stage, the raw MEDS_cohort_dir is only the input to that first resharding stage)
| job: | ||
| name: MEDS_TAB_${name}_${worker}_${now:%Y-%m-%d_%H-%M-%S} | ||
| sweep: | ||
| dir: ${model_log_dir} |
There was a problem hiding this comment.
Ok, my first comment here was right. I don't see where this is defined at the top level. It might be defined nested within a sub-config, but I don't think this will work in that case.
There was a problem hiding this comment.
I think it is possible we are trying to hard to have the configs be "the same" or "similar" between tabularization and modeling. I think separating them out more would be good, because then you can not have the ambiguity of things like the "output cohort dir" meaning an output during tabularization and an input during modeling, etc. I'm not sure what exactly that would look like, but something in there I think would probably be smart.
There was a problem hiding this comment.
Ahhh I see, @teyaberg proposed we call it output_model_dir instead. Let's go with that.
There was a problem hiding this comment.
Maybe the whole hydra block for this and autogluon should be shared?
There was a problem hiding this comment.
And some other params, actually...
There was a problem hiding this comment.
I think this yaml can be used for all model launchers (autogluon, sklearn, and xgboost), and we can add a stage check for autogluon that makes sure users do not apply multirun which will apply the overrides for hydra/sweeper hydra/callbacks and hydra/launcher.
| p: 2 | ||
| metric: "minkowski" | ||
|
|
||
| training_params: |
src/MEDS_tabular_automl/configs/model_launcher/path/default.yaml
Outdated
Show resolved
Hide resolved
|
|
||
| hydra: | ||
| sweeper: | ||
| +params: |
There was a problem hiding this comment.
I'm having issues with having model specific hydra sweeper overrides since they are not in the root model_launcher yaml but are in a child. Any ideas how to resolve this @mmcdermott ?
…, and added an integration test for autogluon
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #90 +/- ##
==========================================
+ Coverage 80.06% 80.09% +0.02%
==========================================
Files 21 21
Lines 1229 1271 +42
==========================================
+ Hits 984 1018 +34
- Misses 245 253 +8 ☔ View full report in Codecov by Sentry. |
Added script input args checks, reduced redundancy in model launcher configs thorugh adding hierarchical defaults. Currently this is broken as the test need to be updated, but I wanted to start a discussion here to check if the config structure looks good for launch_models